@cursoragent review

PR Review: Refactor test suite (#21)

Verdict: Approve with minor nits. This PR delivers on its goal — a clearer testing pyramid, less duplication, and better coverage — without touching production code. The structure is a meaningful improvement over the flat 10-file layout.

What works well

Clear layering. The split into unit/, doctree/, and integration/ matches how the extension actually works: pure string transforms → directive parsing → full build output. Each layer owns behaviors at the right cost level.

Shared infrastructure is the right abstraction. Consolidating build_sphinx, conf templates, wheel setup, and assertion helpers into support.py, helpers.py, and conftest.py removes ~200 lines of copy-paste and makes new tests much cheaper to write.

Parametrization is used well. _asset_href, doctest_to_replay_source, autodoc_bootstrap_source, and doctest scope are expressed as data-driven cases instead of near-duplicate functions. Replacing the full-build html_builder fixture with mock_html_builder is a good speed win.

Redundancy cuts are justified. Removing test_replay_body, the multiline terminator build test, duplicate wheel doctree checks, and the out-of-tree autodoc bootstrap integration test is reasonable — those paths are covered by unit tests plus slimmer integration smoke tests.

Gap-filling tests are targeted. The new tests for :silent:, missing :src:, non-HTML copy_asset_files, vendored JS copy, and startup deduplication close real holes that the old suite missed while chasing 97% coverage.

CI compatibility. PYTHONWARNINGS=error pytest --cov . passes (47 tests, 100% on sphinx_pyrepl_web/__init__.py).

Issues to address (minor)

1. Unused wheel_paths fixture

wheel_paths is defined in conftest.py but never used. Safe to remove in this PR or a quick follow-up.

2. PR description inaccuracy

The summary says "6 integration smoke tests", but there are 10 integration tests:

• test_build_output.py — 6
• test_autodoc_wiring.py — 2
• test_include.py — 1
• test_nested_pages.py — 1

Worth correcting before merge so the before/after table stays trustworthy.

3. Import path fragility

Tests import sibling modules directly:

from support import build_sphinx
from helpers import pyrepl_tag
from fixtures.sources import WIDGET_SOURCE

This works because pytest adds tests/ to sys.path, but it is fragile if tests are collected differently or run outside pytest. Not blocking, but a future hardening step would be either:

• a tests/__init__.py + from tests.support import ..., or
• a single conftest.py that exposes fixtures only and keeps helpers as a proper subpackage.

4. test_copy_asset_files_skips_non_html_builder is indirect

def test_copy_asset_files_skips_non_html_builder():
    app = MagicMock()
    app.builder.format = "latex"
    copy_asset_files(app, None)
    app.builder.outdir.__truediv__.assert_not_called()

This passes because outdir is never accessed when format != "html". It would not catch a regression that touched outdir before the format guard. Low risk given the current code shape; an integration-style check (e.g. no files written under outdir) would be more durable.

Design observations (non-blocking)

Doctree vs full build for autodoc scope — Moving scope tests to sphinx-pytest with a written conf.py (instead of set_conf dict) was the right call. String config values like "autodoc" / "all" do not work through confoverrides.

count_pyrepl_nodes uses substring counting — result.pformat().count("<py-repl") is fine for these fixtures, though a node traversal would be more precise if the doctree format ever embeds that string in non-HTML content.

Some integration overlap remains by design — e.g. test_build_writes_replay_script_from_metadata still asserts script text (x = 2 + 2). That is acceptable because it validates the build pipeline end-to-end, not just conversion logic.

Removed out-of-tree class bootstrap integration — test_autodoc_packages_for_out_of_tree_module is gone; class-level bootstrap import is now covered only by the unit test. That is probably sufficient, but it is the one place where integration wiring for a non-fixture package is no longer exercised.

Summary table (verified)

Recommendation

Merge after:

1. Removing the unused wheel_paths fixture (optional but tidy).
2. Fixing the integration test count in the PR description.

No blocking issues. The refactor is a solid foundation for future extension work — new behaviors should land in unit/doctree first, with integration reserved for wiring and file output.

  

Documentation build overview

📚 sphinx-pyrepl-web | 🛠️ Build #33410884 | 📁 Comparing 10153a8 against latest (931ed8b)

  🔍 Preview build